Skip to content

Parameterize N1QL queries for prepared-statement caching#32

Draft
pimpin wants to merge 5 commits into
masterfrom
pimpin/n1ql-positional-params
Draft

Parameterize N1QL queries for prepared-statement caching#32
pimpin wants to merge 5 commits into
masterfrom
pimpin/n1ql-positional-params

Conversation

@pimpin

@pimpin pimpin commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Replace string-interpolated values with positional parameter placeholders ($1, $2, ...) in all N1QL query paths (n1ql, relation, has_many, query_helper), enabling Couchbase prepared-statement caching by making query strings stable regardless of parameter values.
  • Add a configurable adhoc option (default true) to control whether queries use prepared-statement caching. Set to false to enable caching for repeated queries.

Test plan

  • Existing specs pass (n1ql_spec, relation_spec)
  • New parameterized query specs cover: basic where, NOT, range, hash operators, string passthrough, array IN conditions
  • Verify prepared-statement caching works when adhoc: false against a live Couchbase cluster

🤖 Generated with Claude Code

pimpin and others added 2 commits June 24, 2026 10:17
Replace string-interpolated values with positional parameter placeholders
($1, $2, ...) across n1ql, relation, has_many, and query_helper modules.
This enables Couchbase prepared-statement caching by making query strings
stable regardless of parameter values.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Default adhoc to true in N1ql.config to preserve current behavior
(queries are not cached). When set to false, Couchbase will cache the
parameterized query plan, improving performance for repeated queries.
The option is forwarded to Couchbase::Options::Query in both n1ql and
relation query paths.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces parameterized queries to the Couchbase ORM library to support prepared statements, replacing inline string interpolation with positional parameters across several query-building components. The review feedback highlights a critical N1QL injection vulnerability in nested attribute updates where values are still directly interpolated. Additionally, the reviewer suggests optimizing array parameterization to enable prepared-statement caching for IN queries and adding support for ActiveSupport::TimeWithZone to ensure robust serialization of timezone-aware times.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

end.join(", ") + for_clause
else
"#{key} = #{@model.quote(value)}#{for_clause}"
"#{key} = #{@model.bind(value, params)}#{for_clause}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

In build_update_with_params, when value is a Hash (used for updating nested attributes), the values are directly interpolated into the query string as #{v} without being parameterized or quoted:

if value.is_a?(Hash)
    value.map do |k, v|
        "#{key}.#{k} = #{v}"
    end.join(", ") + for_clause

This not only defeats prepared-statement caching for nested attribute updates but also introduces a N1QL injection vulnerability if user-controlled input is passed to nested attributes.

To fix this, we should parameterize these values using @model.bind(v, params) as well:

if value.is_a?(Hash)
    value.map do |k, v|
        "#{key}.#{k} = #{@model.bind(v, params)}"
    end.join(", ") + for_clause

Comment on lines +7 to +26
def serialize_for_binding(value)
if [DateTime, Time].any? { |clazz| value.is_a?(clazz) }
value.iso8601(@precision || 0)
elsif value.is_a?(Date)
value.to_s
else
value
end
end

def bind(value, params)
if value.is_a?(Array)
"[#{value.map { |v| bind(v, params) }.join(', ')}]"
elsif value.nil?
nil
else
params << serialize_for_binding(value)
"$#{params.length}"
end
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There are two important improvement opportunities in serialize_for_binding and bind:

  1. Array Parameterization (Prepared-Statement Caching): N1QL natively supports passing arrays as a single positional parameter (e.g., name IN $1). The current implementation converts arrays into individual positional parameters (e.g., name IN [$1, $2]), which produces a different query string for every different array length. This completely defeats prepared-statement caching for IN queries (which is one of the main goals of this PR). Passing the array itself as a single parameter keeps the query string identical regardless of array length, enabling prepared-statement caching.

  2. ActiveSupport::TimeWithZone Support: ActiveSupport::TimeWithZone (the default time class in Rails) does not inherit from Time or DateTime, so timezone-aware times will not be serialized to ISO8601 strings and will fail or serialize incorrectly. Adding value.acts_like?(:time) ensures robust support for Rails applications.

            def serialize_for_binding(value)
                if value.is_a?(Array)
                    value.map { |v| serialize_for_binding(v) }
                elsif [DateTime, Time].any? { |clazz| value.is_a?(clazz) } || (value.respond_to?(:acts_like?) && value.acts_like?(:time))
                    value.iso8601(@precision || 0)
                elsif value.is_a?(Date)
                    value.to_s
                else
                    value
                end
            end

            def bind(value, params)
                if value.nil?
                    nil
                else
                    params << serialize_for_binding(value)
                    "$#{params.length}"
                end
            end

pimpin and others added 3 commits June 24, 2026 12:19
Parameterize Hash values in build_update_with_params using bind()
instead of direct string interpolation, which was both a security
vulnerability and defeated prepared-statement caching.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…hing

Instead of expanding arrays into individual parameters (e.g. IN [$1, $2]),
pass the whole array as one parameter (e.g. IN $1). This keeps the query
string stable regardless of array length, enabling Couchbase to cache the
prepared statement for IN queries.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
TimeWithZone does not inherit from Time or DateTime, so it was not
being serialized to ISO8601. Use acts_like?(:time) duck-typing to
handle all time-like objects from Rails.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant